-
Notifications
You must be signed in to change notification settings - Fork 97
feat(metrics): introduce Metrics.flushMetrics #2154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- introduce `Metrics.flushMetrics` as a more powerful version of `flushSingleMetrics` to allow - using defaults by inheriting state e.g. namespace, dimensions and metadata - emitting multiple metrics in one metrics context - refactor `flushSingleMetrics` to use `flushMetrics` - move namespace/service setting from `MetricsFactory` to `EmfMetricsLogger`
6587c5a
to
13eb9d1
Compare
- introduce `Metrics.flushMetrics` as a more powerful version of `flushSingleMetrics` to allow - using defaults by inheriting state e.g. namespace, dimensions and metadata - emitting multiple metrics in one metrics context - refactor `flushSingleMetrics` to use `flushMetrics` - move namespace/service setting from `MetricsFactory` to `EmfMetricsLogger`
13eb9d1
to
537dc90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @humanzz for sending this PR. Overall, it looks good and I left some comments that we need to address before moving forward.
Primarily, can you go back to the issue #2153 and let us know your use-case? We still need to decide whether we want to move forward with inheriting the default configuration of the metrics logger for flushMetrics
and flushSingleMetric
.
powertools-metrics/src/main/java/software/amazon/lambda/powertools/metrics/MetricsFactory.java
Show resolved
Hide resolved
...trics/src/main/java/software/amazon/lambda/powertools/metrics/internal/EmfMetricsLogger.java
Outdated
Show resolved
Hide resolved
...trics/src/main/java/software/amazon/lambda/powertools/metrics/internal/EmfMetricsLogger.java
Outdated
Show resolved
Hide resolved
...trics/src/main/java/software/amazon/lambda/powertools/metrics/internal/EmfMetricsLogger.java
Outdated
Show resolved
Hide resolved
I've addressed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick turnaround @humanzz. Just one more thing that I discovered.
...trics/src/main/java/software/amazon/lambda/powertools/metrics/internal/EmfMetricsLogger.java
Outdated
Show resolved
Hide resolved
...trics/src/main/java/software/amazon/lambda/powertools/metrics/internal/EmfMetricsLogger.java
Outdated
Show resolved
Hide resolved
...s/src/test/java/software/amazon/lambda/powertools/metrics/internal/EmfMetricsLoggerTest.java
Show resolved
Hide resolved
Good catch! It seems that a "property" is simply a key added to the log line while "metadata" becomes part of the Cold start metric (putProperty) {
"_aws": {
"Timestamp": 1751977621635,
"CloudWatchMetrics": [
{
"Namespace": "ServerlessAirline",
"Metrics": [
{
"Name": "ColdStart",
"Unit": "Count"
}
],
"Dimensions": [
[
"Service",
"FunctionName"
]
]
}
]
},
"function_request_id": "b8628984-b745-44bb-8025-247c2da76da1",
"xray_trace_id": "1-686d0e92-396456d519ba8c28174120a4",
"ColdStart": 1,
"Service": "payment",
} Regular metric (putMetadata) {
"_aws": {
"Timestamp": 1751977946689,
"CloudWatchMetrics": [
{
"Namespace": "ServerlessAirline",
"Metrics": [
{
"Name": "CustomMetric1",
"Unit": "Count"
},
{
"Name": "CustomMetric3",
"Unit": "Count",
"StorageResolution": 1
}
],
"Dimensions": [
[
"Service"
]
]
}
],
"function_request_id": "feccb848-47a9-4b32-a16b-73d45d7ad308",
"xray_trace_id": "1-686d0fdb-651f4fc05b57221e726890c0"
},
"CustomMetric1": 1,
"Service": "payment",
"CustomMetric3": 1
} I need to find out the semantic difference – those nuances don't exist in the other runtimes. None of it is searchable in cloudwatch metrics. Besides the location in the json output I don't see any difference. Update: It appears that putMetadata will add key-value pairs to the We should update the |
...trics/src/main/java/software/amazon/lambda/powertools/metrics/internal/EmfMetricsLogger.java
Outdated
Show resolved
Hide resolved
...trics/src/main/java/software/amazon/lambda/powertools/metrics/internal/EmfMetricsLogger.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @humanzz for your continued engagement. Everything looks good so far besides these minor comments. 🚀
The final step after this will be to update the documentation with the new flushMetrics
method including code examples showing developers how to use it. Let me know if you would like to do this – otherwise I am happy to commit the documentation into this PR.
The docs file uses mkdocs and is located here docs/core/metrics.md
.
...trics/src/main/java/software/amazon/lambda/powertools/metrics/internal/EmfMetricsLogger.java
Outdated
Show resolved
Hide resolved
...cs/src/main/java/software/amazon/lambda/powertools/metrics/internal/LambdaMetricsAspect.java
Show resolved
Hide resolved
...s-metrics/src/main/java/software/amazon/lambda/powertools/metrics/internal/MetricsUtils.java
Outdated
Show resolved
Hide resolved
powertools-metrics/src/main/java/software/amazon/lambda/powertools/metrics/Metrics.java
Outdated
Show resolved
Hide resolved
powertools-metrics/src/main/java/software/amazon/lambda/powertools/metrics/Metrics.java
Outdated
Show resolved
Hide resolved
powertools-metrics/src/main/java/software/amazon/lambda/powertools/metrics/Metrics.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Final small comment (you can commit directly from GitHub UI I think).
Co-authored-by: Philipp Page <philipp.page@yahoo.de>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @humanzz for raising the bar. Your attention to detail significantly improved this module!
These changes look good to me. Let me align with the other runtimes and make a merge decision! 🚀
Aligned. I am merging this PR. Congrats @humanzz on this awesome contribution! 🎉 |
Summary
Changes
Metrics.flushMetrics
as a more powerful version offlushSingleMetrics
to allowEmfMetricsLogger.addMetadata
to use emf'sputProperty
flushSingleMetrics
andcaptureColdStartMetric
to useflushMetrics
MetricsUtils
Issue number: #2153
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.